Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

URL-decoding query and form parameters + decoding FormData #392

Merged
merged 7 commits into from
Apr 28, 2024

Conversation

pbrinkmeier
Copy link
Contributor

@pbrinkmeier pbrinkmeier commented Mar 31, 2024

Hiya, first time contributor.

This PR adds

Let me know if this looks good to you guys or if I need to change anything.

closes #321 , closes #170 .

@pbrinkmeier pbrinkmeier mentioned this pull request Mar 31, 2024
3 tasks
@ocramz
Copy link
Collaborator

ocramz commented Mar 31, 2024

thank you @pbrinkmeier ! I'll take a look soon.

@ocramz ocramz self-requested a review April 1, 2024 07:06
Copy link
Collaborator

@ocramz ocramz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Looks good but just a minor comment in ScottySpec, I think we should reuse the implementation from hspec-wai (unless I'm missing something).

Edit: Also could you please add a line in the Changelog describing your changes?

test/Web/ScottySpec.hs Show resolved Hide resolved

describe "formParam" $ do
let
postForm p bdy = request "POST" p [("Content-Type","application/x-www-form-urlencoded")] bdy
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where I got the implementation from.

@pbrinkmeier
Copy link
Contributor Author

I added the changelog entry

@pbrinkmeier
Copy link
Contributor Author

Fixed the conflict in changelog.md

@ocramz ocramz self-requested a review April 7, 2024 06:04
Copy link
Collaborator

@ocramz ocramz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to understand how much does the new function overlap in functionality with formParams and related. Also, I'm not sure using body is a good idea (depends on whether it respects #147 or not)

-- status is set to 400 and an exception is thrown.
--
-- NB : Internally this uses 'body'.
formData :: (FromForm a, MonadIO m) => ActionT m a
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the relationship between this and formParam(s) ?

If the only difference is which form encoding they can handle, we might want to extend the current functions rather than introducing a new one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

formData could be rewritten in terms of formParams, Form and fromForm.

Which shows that yes, it is basically just support for a new encoding.

I initially added this because it was mentioned in #321 but I don't personally think that this is important functionality...

-- NB : Internally this uses 'body'.
formData :: (FromForm a, MonadIO m) => ActionT m a
formData = do
b <- body
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does body preserve the request body across calls to next? see #308

Copy link
Contributor Author

@pbrinkmeier pbrinkmeier Apr 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% sure what the desired behavior is, I assumed this was "right" because of the way jsonData is written :)

Should I add a test here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Internally, both formParams and formData use getBodyAction which I believe leads to consistent behavior unless I'm missing something.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct! (beautiful diagram btw, how did you make it?)

Basically this "lazier" parsing of request bodies (underneath the implementation of files, formParams etc.) is what I introduced in #369 and I think it would make sense for form parameters too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diagram is made using plant UML, basically a text-based UML tool. Getting the call graph from Haskell isn't automatic tho, I did that by hand. Below is the source code for it.

@startuml
skinparam dpi 300

hide members
hide circle

package "Web.Scotty.Action" {
    formParams -down-> "Web.Scotty.Internal.Types.formParamsAndFilesWith"
    formData -down-> body
    body -down-> "Web.Scotty.Internal.Types.envBody"
}

package "Web.Scotty.Internal.Types" {
    formParamsAndFilesWith -left-> envFormDataAction
    envFormDataAction -down-> "Web.Scotty.Body.getFormParamsAndFilesAction"
    envBody -down-> Web.Scotty.Body.getBodyAction
}

note top of Web.Scotty.Internal.Types.envFormDataAction
Initialized by mkEnv
end note

note top of Web.Scotty.Internal.Types.envBody
Initialized by mkEnv
end note

package "Web.Scotty.Body" {
    getFormParamsAndFilesAction -down-> getBodyAction
    getFormParamsAndFilesAction -right-> parseRequestBodyExBS
    parseRequestBodyExBS -down-> "Network.Wai.Parse.sinkRequestBodyEx"
}

package "Network.Wai.Parse" {
    sinkRequestBodyEx -down-> conduitRequestBodyEx
    conduitRequestBodyEx -down-> "Network.HTTP.Types.parseSimpleQuery"
}

package Network.HTTP.Types {
    parseSimpleQuery -right-> parseQuery
    parseQuery -right-> urlDecode
}
@enduml

@ocramz ocramz self-requested a review April 7, 2024 07:47
@pbrinkmeier
Copy link
Contributor Author

I am still not entirely sure how to go forward with formData. Should I just remove it or rewrite it in terms of formParams?

@ocramz
Copy link
Collaborator

ocramz commented Apr 9, 2024

I think rewriting it in terms of formParams would be a great way forward!

@ocramz

This comment was marked as resolved.

Copy link
Collaborator

@ocramz ocramz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Just a changelog conflict to resolve and a comment to clarify why an implementation looks the way it does.

Web/Scotty/Action.hs Show resolved Hide resolved
Copy link
Collaborator

@ocramz ocramz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @pbrinkmeier , all good! 👍

@ocramz ocramz merged commit 9e9c1b4 into scotty-web:master Apr 28, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

URL-decode all inputs URL-decode the textual params coming from the GET query or POST body
2 participants